chore(gates): name the added modules and import paths when an eager closure grows - #2471
Conversation
Size Report
Startup median (7 runs, lower is better):
|
|
Needs changes at 78a7a07:
The |
78a7a07 to
0b2a600
Compare
…ager closure grows The no-growth diagnostic in scripts/__tests__/eager-closure-budgets.ts only named the FIRST newly evaluated module and always advised a dynamic import. On #2423 that sent five reviewers toward the wrong fix when the growth was a small new module that belonged in a module every affected entry already evaluated -- the dynamic-import advice was never coherent for a brand-new module with no old edge to defer. - describeClosureGrowth now lists every added module (bounded to 10), each with the shortest static import route from the entry to it. - describeSharedGrowthHomes runs once after every entry is evaluated: when two or more entries grew by the same added module, it names the modules they already evaluate at the merge-base under that module's own package -- candidate homes, not a verdict. - classifyGrowth's closing advice now states the two common causes (a new static edge, or something that used to load lazily) and the two remedies (give the symbol a home in a module already in the closure, or make the new edge lazy) instead of prescribing one fix. The verdict logic (when an entry is flagged as having grown) is unchanged.
…ed module The cross-entry shared-homes note took every entry with a newly evaluated module, which is not the condition the per-entry rule applies: a closure that swaps one module for another, or shrinks while adding one, has added modules and no growth. `classifyGrowth` passes it, so the aggregate must too -- entries now carry their head closure size and the grouping keeps only the ones whose closure actually grew. Candidate homes are no longer unioned across added modules. Each added module shared by two or more grown entries gets its own block naming those entries with how much each grew and the merge-base modules exactly those entries evaluate, so the label no longer claims a home is common to every failing entry when two independent groups are in play.
f86b2bd to
343fca1
Compare
|
All three addressed in 343fca1 (rebased on main). 1. Aggregate only takes net growth. Regression: 2. One block per added module, no union. Regression: Both regressions were confirmed non-vacuous by re-introducing the two defects (the 3. Complexity. Verification: gate file 588/588 (passes at the merge-base), |
|
The three findings are addressed at 343fca1: only net growth contributes, each added module keeps its own candidate homes, and the complexity gate passes. No remaining code findings. The iOS capture-stall failure also occurs on the base and looks unrelated to this diagnostics-only change; resolve or rerun that check before merging. |
thymikee
added
the
ready-for-human
|
1 participant
What changed
The eager-closure NO-GROWTH gate (
scripts/__tests__/eager-closure-budgets.ts+.test.ts) has good bones — it already names the shortest import chain to a growth —but its message shape sent reviewers the wrong way on #2423. This is a small
diagnostics-only change; the verdict logic (when an entry is flagged as grown) is
identical.
describeClosureGrowthnow lists every newly-added module (bounded to 10, witha count of any more), each with the shortest static import route from the entry —
not just the first one.
describeSharedGrowthHomes, run once after every entry is evaluated: for eachadded module that two or more grown entries share, it prints one block naming
those entries with how much each grew, and the modules exactly those entries already
evaluate at the merge-base, scoped to the added module's own package. Labeled
neutrally as candidate homes, not a verdict.
classifyGrowth's closing advice no longer prescribes one fix. It now states thetwo common causes (a new static edge, or something that used to load lazily) and
the two remedies (give the symbol a home in a module already in the closure, or
make the new edge lazy), leaving the choice to the reader.
Two properties the aggregation keeps, from this PR's review round:
grouping applies the same condition
classifyGrowthdoes. A closure that swapped onemodule for another, or shrank while adding one, has newly evaluated modules and no
growth, so it never appears and never fails the aggregate.
nothing about another group's, so groups stay separate all the way into the message
and the label names the group ("Entries that grew by
<module>…") instead ofclaiming every failing entry.
Motivated by the review rounds on #2423.
Why
#2423's growth was one new tiny constants module reached from several
platform-applefacades through
runner-cache-metadata.ts. The old message named only the firstadded module and always said "move it behind a dynamic import" — which sent five
reviewers toward a dynamic import for a constant, when the right fix was a home in a
module every affected facade already had in its closure. Nothing in any single
entry's own failure could show that the "new" modules across five separate CI
failures were mostly the same one.
Evidence: before / after on a real planted growth
Planted an unused
import './toolchain-probe-budget.ts';(a new, empty module) intopackages/platform-apple/src/runner/runner-cache-metadata.ts— the same shape as#2423: one new module reached by several
platform-applefacades through the samefile. Ran the gate, captured the message, then discarded the plant (
git diffwasback to just the two files below before committing).
Before (one entry's failure —
packages/platform-apple/src/runner/index.ts):That one entry was one of seven that failed the same way —
app-lifecycle-facade,app-resolution-facade,doctor-facade,perf-facade,physical-device-facade,runner-operations-facade, andrunner/index.tsitself — each printing its ownversion of the same chain, with nothing tying them together.
After (same entry):
After, printed once for the whole run (the new aggregation, one block per added
module, re-captured after the review round):
That is the shape #2423 needed: one line pointing at the modules already common to
every affected facade, instead of five people independently reverse-engineering the
same pattern from five separate CI failures. Here there is one added module, so one
block; two independent added modules would print two, each with only its own entries
and homes.
Test plan
pnpm vitest run --project unit-core scripts/__tests__/eager-closure-budgets.test.ts— 588 passed, including the added-module listing (with a route), the shared-homes aggregation (two entries sharing one added module, the no-op single-entry case), and this round's two regressions:an equal-size replacement is not growth, so the aggregate stays silentanddisjoint growth groups keep their own entries and homes in separate blocks. Both were confirmed non-vacuous by re-introducing the two defects (theadded.length > 0filter, and one merged homes list) and watching each fail.pnpm check:fallow --base origin/main— no issues in 2 changed files; every function in the split grouping is under the complexity thresholds.pnpm typecheck— clean (the earlier--untilfailure was fix(cli): declare projectConfig on the scroll --until flag #2472's, now on main).pnpm lint— clean.pnpm check:layering— clean.